[MRG] GUI synaptic gains implementation #918

gtdang · 2024-10-22T18:49:27Z

Added synaptic gains widgets to the GUI.

Changes:

Adjusted Network class methods
2. Renamed update_weights method to set_synaptic_gains for more clarity
3. Added get_synaptic_gains method and test
Added new "Synaptic gains" child tab under the Network tab with widgets for each weight
Updated the _init_network_from_widgets function to update synaptic gains
Added tests for GUI synaptic gains
Corrected spelling error for cell parameters variable

…iguration to align with the tab name.

…unction

…nd updated gui.cell_parameters_widgets calls.

update_weights was too vague.

asoplata · 2024-10-22T19:41:35Z

Did some initial simulation tests using the new tab, and the simulated changes do appear to have the expected effects. Still need to actually review both the code and the tests.

asoplata · 2024-10-22T20:54:24Z

Please see #915 for some discussion on planning this for the GUI.

Also, @dylansdaniels had a great idea: Instead of creating a new Synaptic gains Network tab, we could instead add the values to the top of Connectivity. This way, once we have each individual connection's gain also displayed in the Connectivity tab (currently a TODO), everything for the gains would be in the same location and with less clutter. Additionally, we could also add a button nearby with something like "Apply global synaptic gain changes" which applies a change via one of the four above gains, but then the user can inspect the current, actual value of the gain below on a per-connection basis.

I will try to add the per-connection gain to the Connectivity tab on this branch.

gtdang · 2024-10-22T21:10:44Z

Please see #915 for some discussion on planning this for the GUI.

Also, @dylansdaniels had a great idea: Instead of creating a new Synaptic gains Network tab, we could instead add the values to the top of Connectivity. This way, once we have each individual connection's gain also displayed in the Connectivity tab (currently a TODO), everything for the gains would be in the same location and with less clutter. Additionally, we could also add a button nearby with something like "Apply global synaptic gain changes" which applies a change via one of the four above gains, but then the user can inspect the current, actual value of the gain below on a per-connection basis.

I will try to add the per-connection gain to the Connectivity tab on this branch.

Would that involve a change to the API to set per-connection gain?

asoplata · 2024-10-22T21:19:11Z

I don't think it would need a change to the API, since it should be analogous to changing nc_dict's A_weight value. Related question: is this https://github.com/jonescompneurolab/hnn-core/blob/master/hnn_core/gui/gui.py#L1818-L1819 where the GUI actually applies weight changes from the GUI to the Net object?

asoplata · 2024-10-22T21:26:49Z

Noticed a small bug: Loading in a new network connectivity file adds another full set of the 4 connection gain types:

gtdang · 2024-10-22T21:31:47Z

Yes, but you don't want to be applying the gain and updating the weight there. The nc_dict has a gain parameter. The multiplication should be left to the Network at simulation runtime.

jasmainak

Looks good! Just left tiny nitpicks ... I didn't test though. I am hoping @dylansdaniels or @asoplata took care of that ;)

hnn_core/network.py

jasmainak · 2024-10-23T00:37:19Z

hnn_core/network.py

+def _get_cell_index_by_synapse_type(net):
+    """Returns the indexes of excitatory and inhibitory cells in the network.
+
+    This function extracts the source GIDs (Global Identifiers) of excitatory


"Global Identifiers" isn't too descriptive either ... it's a bit of a misnomer since it's mainly a programming construct. I like to say "cell ID" to be clear.

jasmainak · 2024-10-23T00:41:55Z

hnn_core/network.py

+        return np.concatenate([list(net.connectivity[conn_idx]['src_gids'])
+                               for conn_idx in indices]).tolist()
+
+    e_conns = pick_connection(net, receptor=['ampa', 'nmda'])


Suggested change

e_conns = pick_connection(net, receptor=['ampa', 'nmda'])

picks_e = pick_connection(net, receptor=['ampa', 'nmda'])

I like to use e_conn after the indexing, i.e.:

e_conns = [net.connectivity[p] for p in picks_e]

so it's clear from the variable name that one is an element of net.connectivity and the other is simply an index

hnn_core/gui/gui.py

hnn_core/network.py

jasmainak · 2024-10-23T00:46:45Z

hnn_core/network.py

+        }
+
+        # Retrieve the gain value for each connection type
+        for conn_type, (src_indexes, target_indexes) in conn_types.items():


Suggested change

for conn_type, (src_indexes, target_indexes) in conn_types.items():

for conn_type, (src_idxs, target_idxs) in conn_types.items():

just for sake of consistency

jasmainak · 2024-10-23T00:48:13Z

hnn_core/network.py

+
+        # Retrieve the gain value for each connection type
+        for conn_type, (src_indexes, target_indexes) in conn_types.items():
+            conn_indices = pick_connection(self,


Suggested change

conn_indices = pick_connection(self,

picks = pick_connection(self,

see above comment too ... would try to be consistent with naming

jasmainak · 2024-10-23T00:49:13Z

hnn_core/network.py

+                                           src_gids=src_indexes,
+                                           target_gids=target_indexes)
+
+            if conn_indices:


Suggested change

if conn_indices:

if len(picks) > 0:

to be explicit that picks is a list, not a bool

I think using the truthyness of collections is more concise and pythonic. If we really wanted to be explicit we could also name the variable "picks_list".

But I'll open it up to the rest of the group to define a style guide. @ntolley @asoplata @dylansdaniels

Co-authored-by: Mainak Jas <[email protected]>

jasmainak · 2024-11-07T18:40:05Z

@asoplata @ntolley I see the PR has [MRG] in the title. Please feel free to merge if you're happy with the changes.

asoplata · 2024-11-22T19:30:46Z

hnn_core/network.py

@@ -299,6 +299,40 @@ def pick_connection(net, src_gids=None, target_gids=None,
    return sorted(conn_set)


+def _get_cell_index_by_synapse_type(net):


Is there a reason to put this outside of the Network class as a standalone function if it requires an existing Network?

my general rule is to keep methods of a class minimal, so the structure of the class/object is clear. Just because it takes the object as input doesn't mean it should be a method. The method should have an explicit purpose. For example:

ica = ICA() ica.fit(X) y = ica.predict(X)

once something becomes a method, it's very hard to disentangle it from the class

asoplata · 2024-11-22T19:44:38Z

This looks good, but @dylansdaniels and I have separately discussed that we would like to move these gain settings from their own subtab to the top of Connectivity subtab. I will try to make this change myself, and make a PR to your branch.

gtdang added 12 commits October 22, 2024 10:07

refactor: correct cell_parameter typo

65bae58

feat: added synaptic gain widgets

14f24d3

refactor: renamed connectivity_configuration variable to network_conf…

8911ed3

…iguration to align with the tab name.

refactor: abstracted logic for getting e and i indexes to a private f…

f4b3f6e

…unction

feat: added method for getting synaptic gains values

9c06b59

feat: updated add_synaptic_gain_tab to get gain values from the network

878bc38

test: added synaptic gain widget to init_network_from_widgets calls a…

ace93d4

…nd updated gui.cell_parameters_widgets calls.

refactor: rename update_weights method to set_synaptic_gains.

dbc20f7

update_weights was too vague.

test: added compose check for synaptic gain widgets

94b8108

test: added test for adjusting synaptic gains widget values

3cc0756

refactor: flake8

d46f2a5

test: added test for synaptic gains getter method

05a3cf2

gtdang requested review from ntolley, asoplata and dylansdaniels October 22, 2024 18:49

refactor: updated default dict init in get_synaptic_gains

1fb51cc

gtdang marked this pull request as ready for review October 22, 2024 19:03

gtdang mentioned this pull request Oct 22, 2024

GUI: Enhancements to improve tutorials #910

Open

6 tasks

jasmainak reviewed Oct 23, 2024

View reviewed changes

gtdang and others added 5 commits October 23, 2024 10:45

chore: fix docstr spelling error

d807c6c

Co-authored-by: Mainak Jas <[email protected]>

chore: docstr edit

2348421

Co-authored-by: Mainak Jas <[email protected]>

chore: added docstring

1f9be74

chore: updated docstring

cc6da02

refactor: renamed variables returned from pick connection for clarity

bed5427

jasmainak approved these changes Oct 31, 2024

View reviewed changes

asoplata added this to the 0.4 milestone Nov 11, 2024

asoplata added the hnn-gui HNN GUI label Nov 15, 2024

asoplata modified the milestones: 0.4, 0.5 Nov 15, 2024

asoplata reviewed Nov 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG] GUI synaptic gains implementation #918

[MRG] GUI synaptic gains implementation #918

gtdang commented Oct 22, 2024 •

edited

Loading

asoplata commented Oct 22, 2024

asoplata commented Oct 22, 2024

gtdang commented Oct 22, 2024

asoplata commented Oct 22, 2024

asoplata commented Oct 22, 2024

gtdang commented Oct 22, 2024 •

edited

Loading

jasmainak left a comment

jasmainak Oct 23, 2024

jasmainak Oct 23, 2024

jasmainak Oct 23, 2024

jasmainak Oct 23, 2024

jasmainak Oct 23, 2024

gtdang Oct 23, 2024

jasmainak commented Nov 7, 2024

asoplata Nov 22, 2024

jasmainak Nov 22, 2024

asoplata commented Nov 22, 2024

	e_conns = pick_connection(net, receptor=['ampa', 'nmda'])
	picks_e = pick_connection(net, receptor=['ampa', 'nmda'])

	for conn_type, (src_indexes, target_indexes) in conn_types.items():
	for conn_type, (src_idxs, target_idxs) in conn_types.items():

	conn_indices = pick_connection(self,
	picks = pick_connection(self,

		@@ -299,6 +299,40 @@ def pick_connection(net, src_gids=None, target_gids=None,
		return sorted(conn_set)


		def _get_cell_index_by_synapse_type(net):

[MRG] GUI synaptic gains implementation #918

Are you sure you want to change the base?

[MRG] GUI synaptic gains implementation #918

Conversation

gtdang commented Oct 22, 2024 • edited Loading

asoplata commented Oct 22, 2024

asoplata commented Oct 22, 2024

gtdang commented Oct 22, 2024

asoplata commented Oct 22, 2024

asoplata commented Oct 22, 2024

gtdang commented Oct 22, 2024 • edited Loading

jasmainak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmainak commented Nov 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asoplata commented Nov 22, 2024

gtdang commented Oct 22, 2024 •

edited

Loading

gtdang commented Oct 22, 2024 •

edited

Loading